docs: name concurrent iterations as the cause when the residency budget starves - #39
Closed
emfdavid wants to merge 1 commit into
Closed
docs: name concurrent iterations as the cause when the residency budget starves#39emfdavid wants to merge 1 commit into
emfdavid wants to merge 1 commit into
Conversation
…et starves One InSituDataset owns one ChunkPool and every active iteration shares it, but each holds its own chunk references -- so residency is the sum of their working sets, not the maximum. Sizing the default for ONE iteration is the right default and stays: the engine cannot know how many iterations a caller intends to run, and guessing high would cost memory in the single-iteration case that is almost every case. What was missing is the diagnostic. Starvation advised "raise cache_budget_bytes, or lower batch_size / block_chunks" -- correct, but not actionable when every resident chunk is legitimately referenced and the caller cannot see why. It now names how many iterations share the pool and the pattern that produces that. Owners count from mint to release rather than from their first pin: the iteration that starves BEFORE it can pin anything is exactly the one that needs naming, and counting pin-holders misses it (caught by the test, which failed on the first implementation). docs/tuning.md gains the sizing rule and the reason the default is not multiplied. The auto-sizing site and the docstrings that advertise the shared-pool configuration (buffers.BatchBuffers) carry the caveat, so it is not only in the prose. Refs #38 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WxdT3e62pYT3EVMBF1sA7C
Owner
Author
|
Folding into #37 instead — same review, one PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refs #38 — settles its open question and closes the two items that follow from it.
Stacked on #37 (base is
pool-lifecycle-phase-a); merge that first.One
InSituDatasetowns oneChunkPooland every active iteration shares it —zip(ds.train, ds.val), or twoDataLoaders — but each holds its own chunkreferences, so residency is the sum of their working sets, not the maximum.
The default stays sized for one iteration. The engine cannot know how many you intend
to run, so any automatic multiplier is a guess that silently costs memory in the
single-iteration case, which is almost every case. Running several is an explicit choice,
so sizing for it is the caller's.
What was actually missing is the diagnostic. Starvation advised "raise
cache_budget_bytes, or lower batch_size / block_chunks" — correct, but not actionable
when every resident chunk is legitimately referenced and the caller has no way to see
why. It now names how many iterations share the pool and the pattern that produces that:
Plus
docs/tuning.md§"Several iterations at once multiply the budget" with the sizingrule and why the default is not multiplied, and the caveat at the auto-sizing site and on
the docstring that advertises the shared-pool configuration — so it is not only in prose.
For reviewers
active_ownerscounts from mint to release, not from first pin. My firstimplementation counted owners currently holding a reference, and the test caught it: the
second iteration starves before it can pin anything, so the count read 1 and the note
never fired — missing precisely the case it exists for.
No behavior change beyond the message text: the default budget, the eviction predicate and
the admission path are untouched.
Author attestation
have verified the claims made in this description.
Left unchecked deliberately — drafted by Claude; that box is the human author's.
Checklist
test_starvation_names_concurrent_iterations_as_the_causedrives the real two-iteration deadlock and asserts the count and the pattern
ruff,mypy,pytest -qgreen locally — 299 passed / 20 skipped, against a 298 /20 baseline on the same tree with the same extras (the delta is exactly the new test)
docs/*.md## UnreleasedinCHANGELOG.mdChunkPool→ free-threaded run passes: 70 passed, 1 skipped on 3.13t withPYTHON_GIL=0